Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Non-Embedded App Support #935

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

xplodedthemes
Copy link

@xplodedthemes xplodedthemes commented Aug 7, 2021

Bring back ShopSession service and Shopify Auth from v16
1 Middleware alias "verify.shopify" that will now points to 2 different Middlewares (VerifyShopifyExternal or VerifyShopifyEmbedded) depending on the "appbridge_enabled" config.
@xplodedthemes xplodedthemes changed the title Non embedded Non-Embedded App Support Aug 7, 2021
@gnikyt
Copy link
Owner

gnikyt commented Aug 9, 2021

Thanks for this.

Things I see that I will attempt to work on...

  • AuthorizeShop... is removed in v17, we can somehow massage this into whats existing on v17
  • Removal of CookieHelper... dont think its needed anymore
  • Removal of ShopSession... v17 uses ShopContext injected into the model, somehow massage this in

@gnikyt gnikyt marked this pull request as draft August 9, 2021 13:20
@gnikyt gnikyt added the feature Enhancement to the code label Aug 9, 2021
@gnikyt
Copy link
Owner

gnikyt commented Aug 9, 2021

@xplodedthemes Is there anything else missing, just curious on a couple things as its been a while since I touched non-embedded.

  1. Is there a setting to toggle embedded and non-embedded? Or are you simply swapping the middleware
  2. More with point 1... is there Blade layout modifications to not load appbridge?
  3. Whats the current flow, are you still pointing to /authenticate

@aveeto
Copy link

aveeto commented Aug 9, 2021

@osiset

That's it, I think.

  1. It is all based on the appbridge_enabled option.
  2. Nothing has changed in the views. There was already a condition whether to load app bridge or not within the layout based on the appbridge_enabled option
  3. Yes, I brought back to the authenticate.oauth route from v16.

In v16, there was a login page view that I am currently still using. We can actually use this view for both Embedded and Non-Embedded apps, and it would work fine. The only thing is that, with the non-embed, it will always re-direct to the login page since the session is not saved. It becomes more of an app install page instead of a login page.

@gnikyt
Copy link
Owner

gnikyt commented Aug 9, 2021

I think app_bridge_enabled option is currently removed in master? I'd have to check, if not, then that makes sense and the combined work can begin.

@xplodedthemes
Copy link
Author

@osiset No, the option is still there in master: appbridge_enabled. Note that it's appbridge without underscore.

https://github.com/osiset/laravel-shopify/blob/master/src/resources/config/shopify-app.php

@valdisd
Copy link

valdisd commented Sep 1, 2021

Any updates here? Would love for this to be merged.

@gnikyt
Copy link
Owner

gnikyt commented Sep 8, 2021

@valdisd Not at this time, still trying to get time to work on this.

@valdisd
Copy link

valdisd commented Sep 8, 2021

I tested this and when installing this goes into /shopify/authenticate/oauth redirect loop.

Works if the installation is done with app bridge enabled and disabled afterwards. Will try to locate the problem.

@gnikyt
Copy link
Owner

gnikyt commented Sep 8, 2021

@valdisd I believe the code provided by OP works or partially works, I just need it reworked into the existing code to not have duplication. I outlined some points previously, but feel free to poke around :)

@apurbajnu
Copy link

Any update on this?

@o2b3k
Copy link

o2b3k commented Oct 4, 2021

@osiset Hi any update on this! I really need this feature

@o2b3k
Copy link

o2b3k commented Oct 20, 2021

😏

@Aryanhasanzadeh
Copy link

@osiset
First of all, thank you for the good library you have created
Secondly, please check this new feature soon so that we can continue to use your library in the project

@Aryanhasanzadeh
Copy link

@xplodedthemes

I applied your commits manually in the project, but I still have the following error, you can help me ?

impo

@t08-apporio
Copy link

hi @xplodedthemes composer installed your fork. it's currently stuck in a redirect loop. The url in question is authenticate/oauth. I updated appbridge_enabled to false and also disabled the embedded app option in app setup. Any idea what the issue could be?

@Aryanhasanzadeh were you able to make any progress?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature Enhancement to the code help-wanted Contributor help would be nice!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants